Closed
Bug 862798
Opened 11 years ago
Closed 11 years ago
About:home "Saved for later" panel (reading list)
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 26
People
(Reporter: ibarlow, Assigned: Margaret)
References
Details
(Whiteboard: fixed-fig)
Attachments
(3 files, 1 obsolete file)
This is the bug to track work on the new "saved for later" panel on about:home, which will first be used to display the user's Reading List. Note that we will be looking at ways to display the Reading list in a more useful and editorial way, instead of the current title/URL implementation. The new list should display * Title * Intro * Picture, if available * Length of article, in minutes Mockups to follow shortly.
Reporter | ||
Comment 1•11 years ago
|
||
Reporter | ||
Comment 2•11 years ago
|
||
This bug should include work for: * Displaying a more editorial list of articles (using twitter cards, for example) * Calculating the length of the article, in minutes
Updated•11 years ago
|
Whiteboard: good-first-bug-fig
Updated•11 years ago
|
Summary: About:home "Saved for later" panel → About:home "Saved for later" panel (reading list)
Comment 3•11 years ago
|
||
(In reply to Ian Barlow (:ibarlow) from comment #2) > This bug should include work for: > > * Displaying a more editorial list of articles (using twitter cards, for > example) > * Calculating the length of the article, in minutes Open a new bug for any new features. This bug is about feature parity with current "reading list"
Updated•11 years ago
|
Priority: -- → P1
Assignee | ||
Comment 4•11 years ago
|
||
I can take this (implementing feature parity with the current reading list).
Assignee: nobody → margaret.leibovic
OS: Mac OS X → Android
Hardware: x86 → ARM
Assignee | ||
Comment 5•11 years ago
|
||
I feel like I did a bit too much copy/pasting to get to this patch, but it gets the job done. It's probably not worth factoring some of the copy/pasted stuff out into some shared superclass, since we'll be customizing the views of this page later. I still don't fully understand how CursorLoaders work, so I very well may have done something wrong :) One known issue here is that I need to add that "Articles you save for later go here" message when the list is empty, but I figured that could be done in a follow-up.
Attachment #770495 -
Flags: review?(sriram)
Attachment #770495 -
Flags: review?(bnicholson)
Assignee | ||
Comment 6•11 years ago
|
||
Comment 7•11 years ago
|
||
Comment on attachment 770495 [details] [diff] [review] patch Review of attachment 770495 [details] [diff] [review]: ----------------------------------------------------------------- Looks really good. Only thing is the use of SimpleCursorAdapter. I would like to switch that to CursorAdapter, and see a version. Other than that, everything else is fine. r- just to see a version with CursorAdapter. ::: mobile/android/base/home/ReadingListPage.java @@ +42,5 @@ > + // Callbacks used for the reading list and favicon cursor loaders > + private CursorLoaderCallbacks mCursorLoaderCallbacks; > + > + // Inflater used by the adapter > + private LayoutInflater mInflater; This is not needed. @@ +60,5 @@ > + throw new ClassCastException(activity.toString() > + + " must implement HomePager.OnUrlOpenListener"); > + } > + > + mInflater = (LayoutInflater) activity.getSystemService(Context.LAYOUT_INFLATER_SERVICE); Not needed. @@ +135,5 @@ > + return BrowserDB.getBookmarksInFolder(getContext().getContentResolver(), Bookmarks.FIXED_READING_LIST_ID); > + } > + } > + > + private class ReadingListAdapter extends SimpleCursorAdapter { Please use a CursorAdapter. @@ +144,5 @@ > + @Override > + public View getView(int position, View convertView, ViewGroup parent) { > + final TwoLinePageRow row; > + if (convertView == null) { > + row = (TwoLinePageRow) mInflater.inflate(R.layout.home_item_row, mList, false); This could be a direct call like, row = new TwoLinePageRow(parent.getContext()); Hence you won't need an mInflater. And that part will be in newView(). bindView() will take care of coversion. @@ +150,5 @@ > + row = (TwoLinePageRow) convertView; > + } > + > + final Cursor c = getCursor(); > + if (!c.moveToPosition(position)) { This won't be needed when CursorAdapter is used. Always the cursor is moved to required position. @@ +160,5 @@ > + return row; > + } > + } > + > + private class CursorLoaderCallbacks implements LoaderCallbacks<Cursor> { A small documentation may be?
Attachment #770495 -
Flags: review?(sriram) → review-
Comment 8•11 years ago
|
||
Comment on attachment 770495 [details] [diff] [review] patch Review of attachment 770495 [details] [diff] [review]: ----------------------------------------------------------------- r+ with Sriram's comments addressed. ::: mobile/android/base/home/ReadingListPage.java @@ +47,5 @@ > + > + // On URL open listener > + private OnUrlOpenListener mUrlOpenListener; > + > + public ReadingListPage() { } This is unnecessary since Java automatically creates a default constructor if none are specified.
Attachment #770495 -
Flags: review?(bnicholson) → review+
Assignee | ||
Comment 9•11 years ago
|
||
Updated to address comments.
Attachment #770495 -
Attachment is obsolete: true
Attachment #770541 -
Flags: review?(sriram)
Comment 10•11 years ago
|
||
Comment on attachment 770541 [details] [diff] [review] patch v2 Review of attachment 770541 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: mobile/android/base/home/ReadingListPage.java @@ +44,5 @@ > + > + // On URL open listener > + private OnUrlOpenListener mUrlOpenListener; > + > + public ReadingListPage() { } Please remove this.
Attachment #770541 -
Flags: review?(sriram) → review+
Assignee | ||
Comment 11•11 years ago
|
||
https://hg.mozilla.org/projects/fig/rev/7f1cfc8c84e8
Whiteboard: good-first-bug-fig → fixed-fig
Assignee | ||
Comment 12•11 years ago
|
||
Backed out for build failures: https://hg.mozilla.org/projects/fig/rev/35f62a290d4e I'll look into fixing and relanding.
Whiteboard: fixed-fig
Assignee | ||
Comment 13•11 years ago
|
||
Turned out I botched resolving a conflict in the Makefile. Fixed: https://hg.mozilla.org/projects/fig/rev/b689792b6fd2
Whiteboard: fixed-fig
Comment 14•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b689792b6fd2
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•